[madvise] Adapted the code to take care of variable names as we trans… #9
Conversation
…ition from 6.13 kernel baseline to 6.15
…ved json files. updated the single and batch scripts accordingly
There was a problem hiding this comment.
Pull Request Overview
This PR updates madvise tests and scripts to support kernel 6.15 sysfs parameter names, consolidates IAA configuration into a single script, and removes dependencies on the old enable_kernel_iaa.sh.
- Add fallback to
reclaim-batchsizewhen readingcompress-batchsizein tests. - Refactor IAA configuration: replace
enable_kernel_iaa.shcalls withenable_iaa.shand handle new sysfs variables. - Update bpftrace and other collection scripts to use new kernel parameter names (
vm.compress-batchsize→vm.reclaim-batchsize).
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/madvise/madvise_test.c | Fallback from compress-batchsize to reclaim-batchsize and commented out perror calls |
| tests/madvise/enable_kernel_iaa.sh | Add conditional writes for g_comp_wqs_per_iaa, g_wqs_per_iaa, and g_consec_descs_per_gwq |
| tests/madvise/enable_iaa.sh | New unified IAA configuration script replacing enable_kernel_iaa.sh |
| tests/madvise/collect_single_page.sh | Switch to enable_iaa.sh and simplify compression algorithm list |
| tests/madvise/collect_bpftraces.sh | Add sysctl fallback for reclaim-batchsize and adjust error messages |
| tests/madvise/collect_batch.sh | Replace enable_kernel_iaa.sh with enable_iaa.sh |
Comments suppressed due to low confidence (1)
tests/madvise/collect_bpftraces.sh:89
- The error message still references 'compress-batchsize' but this command sets 'reclaim-batchsize'. Update the message to 'Failed to set reclaim-batchsize'.
sysctl vm.reclaim-batchsize=${CBATCH} || handle_error "Failed to set compress-batchsize"
| @@ -123,6 +123,10 @@ int main(int argc, char **argv) | |||
| // Replace the newline character with a null terminator | |||
| compressor[newline_pos] = '\0'; | |||
| int cbatch = read_sysfs("/proc/sys/vm/compress-batchsize"); | |||
| // NOTE: This is a temporary solution as we transition to the new kernel | |||
| if ( cbatch == EXIT_FAILURE ) { | |||
There was a problem hiding this comment.
Using EXIT_FAILURE as a sentinel for read_sysfs failure can collide with a valid value of 1. Consider returning a distinct error code (e.g., -1) or leveraging errno to distinguish failures unambiguously.
| if ( cbatch == EXIT_FAILURE ) { | |
| if ( cbatch == -1 ) { |
Copilot uses AI. Check for mistakes.
| @@ -33,7 +33,7 @@ long read_sysfs(char *path ){ | |||
| FILE* fp = fopen(path, "r"); | |||
| char buf[128]; | |||
| if (!fp) { | |||
| perror("Couldn't open file"); | |||
| //perror("Couldn't open file"); | |||
There was a problem hiding this comment.
[nitpick] Commented-out perror calls may clutter the code and confuse readers. Consider removing these lines or replacing them with a clear logging statement explaining why errors are suppressed.
| //perror("Couldn't open file"); | |
| fprintf(stderr, "Error: Couldn't open file %s. Suppressing perror output.\n", path); |
Copilot uses AI. Check for mistakes.
| if [ -f /sys/bus/dsa/drivers/crypto/g_comp_wqs_per_iaa ];then | ||
| echo 1 > /sys/bus/dsa/drivers/crypto/g_comp_wqs_per_iaa || handle_error "did not set g_comp_wqs_per_iaa" | ||
| elif [ -f /sys/bus/dsa/drivers/crypto/g_wqs_per_iaa ];then | ||
| echo 1 > /sys/bus/dsa/drivers/crypto/g_comp_wqs_per_iaa || handle_error "did not set g_wqs_per_iaa" |
There was a problem hiding this comment.
In the elif branch you are writing to g_comp_wqs_per_iaa instead of g_wqs_per_iaa. Update the path to /sys/bus/dsa/drivers/crypto/g_wqs_per_iaa to match the intended file.
| echo 1 > /sys/bus/dsa/drivers/crypto/g_comp_wqs_per_iaa || handle_error "did not set g_wqs_per_iaa" | |
| echo 1 > /sys/bus/dsa/drivers/crypto/g_wqs_per_iaa || handle_error "did not set g_wqs_per_iaa" |
Copilot uses AI. Check for mistakes.
Adapted the code to transition from kernel 6.13 to 6.15 baseline
Refactored iaa configuration script to a single one without any other dependencies